BUG: rocket with a late-starting thrust curve never leaves the rail (#411) - #1085
Conversation
|
@RocketPy-Team Could a maintainer please approve the pending GitHub Actions workflows for this fork-based PR? The runs are waiting for approval before they can start. Thank you! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1085 +/- ##
===========================================
+ Coverage 82.18% 82.53% +0.34%
===========================================
Files 122 128 +6
Lines 16355 16556 +201
===========================================
+ Hits 13441 13664 +223
+ Misses 2914 2892 -22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Gui-FernandesBR
left a comment
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
Approving. I reproduced both the bug and the fix, and checked the thing that actually matters for a change inside __setup_phase_time_nodes — whether ordinary motors move.
The bug is real and total. I reverted only rocketpy/simulation/flight.py to develop, kept your test, and it fails exactly as you describe:
assert flight.out_of_rail_time > 8.0
E assert 0 > 8.0
Ordinary motors are byte-for-byte unaffected. Two rockets with the same total impulse, differing only in start time, printed at full precision:
| develop | with the fix | |
|---|---|---|
t=0 out_of_rail_time |
0.3289354306801905 |
0.3289354306801905 |
t=0 apogee |
12904.559694188314 |
12904.559694188314 |
t=0 function_evaluations |
2368463 |
2368463 |
t=8 out_of_rail_time |
0 |
8.329156434166551 |
t=8 apogee |
0 |
12904.497507159389 |
t=8 function_evaluations |
9 |
1953325 |
The t=0 column is identical down to the solver's function-evaluation count, which is the strongest evidence I could get that the burn_start > 0 guard means ordinary motors take exactly the same path. And function_evaluations = 9 on develop for the delayed case is a nice confirmation of the diagnosis: the solver really does take one enormous step and stop.
The delayed flight also lands on the same apogee as the t=0 one to within 0.06 m, which is what you would expect from the same impulse applied 8 s later.
Also checked: EmptyMotor is constructed with burn_time=1, so its burn_start_time is 0 and it falls into the guard rather than the new branch. The getattr(motor, "burn_start_time", 0) or 0 is defensive but harmless.
Local runs: tests/unit 1848 passed / 16 skipped; tests/unit/simulation/test_flight.py + tests/integration/simulation/test_flight.py 79 passed / 9 skipped; ruff check and ruff format --check clean; pylint rocketpy/simulation/flight.py 10.00/10.
On the Codecov rename — I was ready to ask you to drop it as unrelated scope, and then I looked into it. .github/workflows/codecov.yml is not a workflow at all; it is Codecov's coverage: config sitting in the workflows directory, so Actions has been trying to execute it and failing on every single run since #500 introduced it. Moving it to .codecov.yml is where Codecov documents it and makes that noise go away. Confirmed the rename is content-identical (diff is empty), so keep it — it is a real fix, and worth calling out in the PR body rather than leaving it looking incidental.
Last note: this now has its first green run — all six matrix legs plus Codecov — after #1084 landed and I updated the branch. Every prior run on this PR died in the un-isolated integration step, which is why it never had one.
2 participants
Pull request type
Checklist
ruff check,ruff format --check; pylint 10/10 on the touched file)tests/unit/simulation/test_flight.py, 56 passed)CHANGELOG.mdupdatedCurrent behavior
Fixes #411. A motor whose thrust curve starts at
t > 0(e.g.burn_time=(8, 20)) never lifts the rocket off the rail — the flight stays at(0, 0, 0)without_of_rail_time == 0.Root cause (as the issue originally guessed): the rail phase's only time nodes are
[t=0, max_time]. With no thrust att=0the rocket is stationary, so LSODA — whosemax_stepdefaults toinf— takes one huge step that jumps clean over the burn. The thrust is never sampled and the rocket never accelerates. A normal motor accelerates att=0, forcing small steps, so it only bites for late-starting curves.Minimal repro (same rocket and total impulse, only the start time differs):
New behavior
__setup_phase_time_nodesnow forces a solver time node at the motor's ignition and burn-out wheneverburn_start_time > 0, so the burn is always sampled. With the fix the delayed case flies identically to thet=0case, just shifted:The change is guarded to
burn_start_time > 0, so ordinary motors (which ignite att=0) hit none of the new code and are byte-for-byte unaffected — confirmed by the unchangedt=0result above and the existing flight tests staying green.Breaking change
Additional information
Adds
test_flight_with_delayed_burn_leaves_railintests/integration/simulation/test_flight.py; it asserts aburn_time=(8, 20)rocket leaves the rail after ignition and reaches altitude. It fails ondevelopand passes with this change.Closes #411